-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check if last pressed key is part of hotkey combination #120
Check if last pressed key is part of hotkey combination #120
Conversation
Just a note: fixes #119 |
Thanks @maxbergmark! I'll make sure to review this over the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far! Current thoughts so far. Will continue reading later!
@friendlymatthew All good suggestions, I fixed them immediately. I also noticed that #127 causes conflicts with my changes. I changed my code to also use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! One minor comment, but otherwise this looks good to merge
@friendlymatthew Good suggestion, I'll clean it up. A quick note: I got a bit worried that I'd broken something: Due to #127, there might need to be some updates to the documentation. I noticed that all my hotkeys that were set up using alphanumeric keys weren't working. After some digging, it seems that the reason is that the event code and the event key are different. While the event code is e.g.
|
|
||
#[cfg(not(feature = "ssr"))] | ||
fn clean_key(event: &web_sys::KeyboardEvent) -> String { | ||
match event.key().as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@friendlymatthew When I merged in the changes from #121, I noticed that this is the only place where event.key()
is referenced. So if you've come up with a feature flag for switching between event.code()
and event.key()
, I'll add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxbergmark thanks for flagging! Would you be interested in attacking #131? Work is taking up all my time, so I'm afraid I won't have time to get this done ASAP.
On another note, would you be interested in joining as a maintainer? You'd be requested for reviews and help steer this project. Let me know! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@friendlymatthew I made an attempt at solving #131. I added a feature called event_key
and just used it in the clean_key
function. That should do the job, but I'd be happy to fix any issues that I might have overlooked (maybe the spacebar handling?). The name of the feature flag was just something I made up, I'll change it if you have a better suggestion.
I'm honored to be invited as a maintainer of this repo, but right now I can't find the time that it would require. I think you're doing great work, and I'll happily keep using the repo, and contribute in discussions and issues when I have some time left over.
@all-contributors please add @maxbergmark for code |
I've put up a pull request to add @maxbergmark! 🎉 |
Hey @maxbergmark, what do you say we rollback on these two commits, and make these their own PRs? That way we can get this merged in at the time 1afdfb5 |
@friendlymatthew Sounds like a good plan, it's better practice to have separate PRs. |
Great, let me know when you do that. I'll take a final look and then should be good to merge. |
This reverts commit f721c9b.
This reverts commit f1e5410.
@friendlymatthew I reverted the past two commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LET'S GOOOOOOOOO! So awesome to see this over the line @maxbergmark!
Just a note, we'll need to immediately handle the .key()
calling convention. #135
Also, thank you for spending time on this! |
Thank you for approving this! If you merge it (I don't have write access) you could re-use the logic for using either the key or code: #[cfg(not(feature = "ssr"))]
fn clean_key(event: &web_sys::KeyboardEvent) -> String {
if cfg!(feature = "FEATURE_NAME") {
match event.key().as_str() {
" " => "spacebar".to_string(),
key => key.to_lowercase(),
}
} else {
event.code()
}
} |
Hmm, I sent an invitation to have write access to this crate for both you and @rkimoakbioinformatics. Seems like your invite got expired, I just resent it, can you check your email/Github notifications? |
No description provided.